NGINX Configs to Reference Remote External Files #1389

Akshay2191 · 2025-11-12T15:50:37Z

Extends the ConfigApply capability to manage NGINX configurations that reference external resources hosted at remote URLs. The agent now handles downloading of these files into the NGINX configuration directory before applying the new configuration.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
I have run make install-tools and have attached any dependency changes to this pull request
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
If applicable, I have updated any relevant documentation (README.md)
If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

codecov · 2025-11-12T16:03:00Z

Codecov Report

❌ Patch coverage is 89.82301% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.38%. Comparing base (b1947ec) to head (4a2b917).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/file/file_manager_service.go	87.73%	12 Missing and 8 partials ⚠️
internal/config/config.go	95.23%	1 Missing and 1 partial ⚠️
internal/file/file_service_operator.go	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1389      +/-   ##
==========================================
+ Coverage   86.31%   86.38%   +0.06%     
==========================================
  Files         102      102              
  Lines       12603    12798     +195     
==========================================
+ Hits        10878    11055     +177     
- Misses       1249     1260      +11     
- Partials      476      483       +7

Files with missing lines	Coverage Δ
internal/config/defaults.go	`100.00% <ø> (ø)`
internal/config/flags.go	`100.00% <ø> (ø)`
internal/config/types.go	`87.65% <ø> (ø)`
internal/model/config.go	`90.47% <ø> (ø)`
internal/file/file_service_operator.go	`77.32% <95.23%> (+1.18%)`	⬆️
internal/config/config.go	`87.60% <95.23%> (+0.29%)`	⬆️
internal/file/file_manager_service.go	`78.77% <87.73%> (+3.38%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1947ec...4a2b917. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aphralG · 2025-11-20T11:16:33Z

internal/config/config.go

+	fs.Duration(
+		ClientFileDownloadTimeoutKey,
+		DefClientFileDownloadTimeout,
+		"Timeout value in seconds, to downloading file for config apply.",


Suggested change

"Timeout value in seconds, to downloading file for config apply.",

"Timeout value in seconds, for downloading a file during a config apply.",

aphralG · 2025-11-20T11:16:59Z

internal/config/config.go

+	fs.String(
+		ExternalDataSourceProxyUrlKey,
+		DefExternalDataSourceProxyUrl,
+		"Url to the proxy service to fetch the external file.",


Suggested change

"Url to the proxy service to fetch the external file.",

"Url to the proxy service for fetching external files.",

aphralG · 2025-11-20T11:20:50Z

internal/config/config_test.go

 		})
 	}
 }
+


Can you add a test for external Data source and add the external data source to TestResolveConfig() which checks all the values that can be set in the agent config

aphralG · 2025-11-20T11:21:54Z

internal/file/file_manager_service.go

-	filesMutex            sync.RWMutex
+	currentFilesOnDisk     map[string]*mpi.File // key is file path
+	previousManifestFiles  map[string]*model.ManifestFile
+	newExternalFileHeaders map[string]DownloadHeaders


Suggested change

newExternalFileHeaders map[string]DownloadHeaders

externalFileHeaders map[string]DownloadHeaders

aphralG · 2025-11-20T11:25:01Z

internal/file/file_manager_service.go

 			fileDiff[fileName] = modifiedFile

-			continue
 			// if file currently exists and file hash is different, file has been updated


Why was the continue removed ?

aphralG · 2025-11-20T11:34:09Z

internal/file/file_manager_service.go


 			continue
-		case model.Delete, model.Update:
+		case model.Delete, model.Update, model.ExternalFile:


With the external files are the files still considered updated, added or deleted? Just wondering instead should it be a field added to the file rather than an action this might fix a few of the things the linter had an issue with which required the checking of Adding and updating in the switch statement

aphralG · 2025-11-20T14:11:51Z

internal/file/file_manager_service.go

+		headers.ETag = resp.Header.Get("ETag")
+		headers.LastModified = resp.Header.Get("Last-Modified")
+	case http.StatusNotModified:
+		slog.InfoContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)


Suggested change

slog.InfoContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)

slog.DebugContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)

aphralG · 2025-11-20T14:12:45Z

internal/file/file_manager_service.go

+	}
+}
+
+func isWildcardMatch(hostname, pattern string) bool {


Can the naming of this be changed

aphralG · 2025-11-20T14:13:25Z

internal/file/file_manager_service.go

+		return false
+	}
+
+	for _, pattern := range allowedDomains {


Suggested change

for _, pattern := range allowedDomains {

for _, domain := range allowedDomains {

aphralG · 2025-11-20T14:14:33Z

internal/file/file_manager_service_test.go

 	"github.com/stretchr/testify/require"
 )

+type fakeFSO struct {


We already have a fake file operator in agent/internal/file/filefakes/fake_file_operator.go

dhurley · 2025-11-19T10:08:27Z

internal/config/config.go

+	fs.Duration(
+		ClientFileDownloadTimeoutKey,
+		DefClientFileDownloadTimeout,
+		"Timeout value in seconds, to downloading file for config apply.",


Suggested change

"Timeout value in seconds, to downloading file for config apply.",

"Timeout value in seconds, to download a file during a config apply request.",

dhurley · 2025-11-19T10:12:21Z

internal/config/config.go

+
+	for _, domain := range domains {
+		if strings.ContainsAny(domain, "/\\ ") || domain == "" {
+			slog.Error("domain is not specified in allowed_domains")


no need to log error here if you are returning the error anyways

dhurley · 2025-11-19T10:13:19Z

internal/config/config.go

+	}
+
+	for _, domain := range domains {
+		if strings.ContainsAny(domain, "/\\ ") || domain == "" {


Is there a better way to validate if the string is a valid domain name or not? Maybe there is a regex we could use?

dhurley · 2025-11-19T10:15:26Z

internal/config/defaults.go

 	DefLibDir = "/var/lib/nginx-agent"
+
+	DefExternalDataSourceProxyUrl = ""
+	DefExternalDataSourceMaxBytes = 100 * 1024 * 1024


can you add a comment to say what the default size is in MB

dhurley · 2025-11-19T16:33:04Z

internal/file/file_service_operator.go

 	return fso.sendUpdateFileStream(ctx, fileToUpdate, fso.agentConfig.Client.Grpc.FileChunkSize)
 }

+func (fso *FileServiceOperator) RenameExternalFile(


instead of duplicating the RenameFile function can we update the RenameFile function to just rename the file and have the hash validation in a separate function like this:

func (fso *FileServiceOperator) RenameAndValidateFile( ctx context.Context, source, destination string, ) error { err := fso.RenameFile(ctx, source, destination) if err != nil { return err } return fso.validateFileHash(desination, hash) }

dhurley · 2025-11-19T16:40:30Z

internal/file/file_manager_service.go

 	executePerm = 0o111
 )

+type DownloadHeaders struct {


Suggested change

type DownloadHeaders struct {

type DownloadHeader struct {

dhurley · 2025-11-19T16:43:24Z

internal/file/file_manager_service.go

 			modifiedFile.Action = model.Update
 			fileDiff[fileName] = modifiedFile
 		}
+		if modifiedFile.File.GetExternalDataSource() != nil || currentFile.GetExternalDataSource() != nil {


can you move this if statement up to just after line 404? It will save you from doing unnecessary os.Stat

dhurley · 2025-11-21T10:00:24Z

internal/file/file_manager_service.go

+func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, fileAction *model.FileCache,
+	tempFilePath string,
+) error {


Suggested change

func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, fileAction *model.FileCache,

tempFilePath string,

) error {

func (fms *FileManagerService) downloadExternalFile(ctx context.Context, fileAction *model.FileCache,

filePath string,

) error {

dhurley · 2025-11-21T10:05:58Z

internal/file/file_manager_service.go

+		ctx,
+		content,
+		path,
+		"0600",


the file permissions should come from the file meta in the File proto message

Akshay2191 added 2 commits November 12, 2025 15:29

third party config support

ae7fa14

lint fix

14f1190

Akshay2191 requested a review from a team as a code owner November 12, 2025 15:50

github-actions bot added the chore Pull requests for routine tasks label Nov 12, 2025

Merge branch 'main' into external-file-mgmt-integration

84b2310

Akshay2191 added 8 commits November 13, 2025 12:12

Added a generic file timeout

d31b2df

merging main to resolve conflicts

b7a5c70

added generic timeout to create connection

501e08f

added uT

b7eaef7

Added UT

a83e45b

fix lint issues

529c839

Added UT for Proxy URL

2a51d22

increasing UT coverage

f22834f

Akshay2191 self-assigned this Nov 19, 2025

updated Mock management to test external file config apply

4a2b917

aphralG reviewed Nov 20, 2025

View reviewed changes

dhurley reviewed Nov 21, 2025

View reviewed changes

	"Timeout value in seconds, to downloading file for config apply.",
	"Timeout value in seconds, for downloading a file during a config apply.",

	"Url to the proxy service to fetch the external file.",
	"Url to the proxy service for fetching external files.",

	newExternalFileHeaders map[string]DownloadHeaders
	externalFileHeaders map[string]DownloadHeaders

	slog.InfoContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)
	slog.DebugContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)

	for _, pattern := range allowedDomains {
	for _, domain := range allowedDomains {

	"Timeout value in seconds, to downloading file for config apply.",
	"Timeout value in seconds, to download a file during a config apply request.",

NGINX Configs to Reference Remote External Files #1389

Are you sure you want to change the base?

NGINX Configs to Reference Remote External Files #1389

Uh oh!

Conversation

Akshay2191 commented Nov 12, 2025

Checklist

Uh oh!

codecov bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 12, 2025 •

edited

Loading